refactor: converge Maestro assertion waits#625
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4235852dbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Review — converge Maestro assertion waits
This is a good correctness and convergence change. Sharing one visibility sampler between assertVisible/assertNotVisible removes a duplicated snapshot-resolution path, and routing extendedWaitUntil.notVisible through the not-visible assertion is actually more Maestro-faithful than the old wait(timeout) + is hidden: native is hidden fails on a selector that doesn't resolve at all, whereas Maestro treats an absent (or overlay-blocked) target as "not visible → pass." The inline comments explaining why native wait/is can't replace these loops are exactly the rationale that should live next to this code.
I checked the behavior-preservation points and they hold:
- assertVisible default timeout stays 5000ms (now via
defaultTimeoutMs). - Plain
assertNotVisiblekeeps the 3000ms default (defaultAssertNotVisibleTimeoutMs), whileextendedWaitUntil.notVisiblenow forwards its ownreadTimeoutMs(..., 30000). So the long extended-wait window is preserved and the bare command's stability window is unchanged. - Infrastructure failures (snapshot capture/parse) still short-circuit instead of being counted as "hidden."
One behavioral nuance worth a sanity check: the new waitedMs >= timeoutMs exit lets assertNotVisible pass on a single hidden sample once the (possibly very short) timeout is reached, where the default path still wants hiddenSamples >= 2. That's the right trade-off for explicit short extendedWaitUntil timeouts, but please confirm there's no flow relying on the old "always require 2 stable hidden samples" guarantee with a sub-poll-interval timeout — the new timeout: 1 test passes precisely because of this branch.
Looks good to merge.
Generated by Claude Code
|
1 participant
Summary
Refactors Maestro visibility assertions so
assertVisibleandassertNotVisibleshare one Maestro visibility sampler while preserving the nativeisvisibility predicate semantics already used by the resolver.Routes
extendedWaitUntil.notVisiblethrough the Maestro not-visible assertion path with its timeout, so absent targets pass consistently instead of falling through nativeis hiddenselector-not-found behavior.Documents why
assertVisibleandwaitForAnimationToEndkeep Maestro-specific polling where nativewait/isare not semantically equivalent.Touched files: 5.
Validation
Focused Maestro assertion/replay tests passed, including the full focused replay file outside the sandbox for its local HTTP fixture. Typecheck passed with the pinned Node/pnpm path.
pnpm formatpassed with the pinned Node/pnpm path; unrelated formatter churn was restored.Known gaps: no broad provider-scenario sweep; this change is scoped to assertion/wait convergence and preserves existing Maestro polling where native semantics differ.